Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: make OAuth2 configuration optional #1057

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

feat: make OAuth2 configuration optional #1057

wants to merge 2 commits into from
+159 −60

Conversation

svor
Copy link
Contributor

@svor svor commented Dec 3, 2024 •
edited
Loading

This pull request introduces several changes to enable and check OAuth2 support in the application. The key changes include adding methods to check OAuth2 status, modifying the security configuration, and updating the web UI to reflect OAuth2 status.

In case when OAuth2 is not configured Publish and Log In buttons are hidden and we have like readonly mode:

Related issue: #1041

Web version:

screenshot-che-dogfooding_apps_che-dev_x6e0_p1_openshiftapps_com-2024_12_03-14_09_53

Mobile version:

photo_2024-12-03_14-15-18

What I did to test how it works

  1. Remove from openvsx/server/src/dev/resources/application.yml this block:
  security:
      oauth2:
        client:
          registration:
            eclipse:
              authorization-grant-type: authorization_code
              redirect-uri: http://localhost/login/oauth2/code/eclipse
              scope: openvsx_publisher_agreement, profile
          provider:
            eclipse:
              authorization-uri: https://accounts.eclipse.org/oauth2/authorize
              token-uri: https://accounts.eclipse.org/oauth2/token
              user-info-uri: https://accounts.eclipse.org/oauth2/UserInfo
              user-name-attribute: name
              user-info-authentication-method: header
  1. Remove from openvsx/server/scripts/generate-properties.sh part that generates OAuth props:
# Set the GitHub OAuth client id and client secret
echo "spring.security.oauth2.client.registration.github.client-id=${GITHUB_CLIENT_ID:-none}" >> $OVSX_APP_PROFILE
echo "spring.security.oauth2.client.registration.github.client-secret=${GITHUB_CLIENT_SECRET:-none}" >> $OVSX_APP_PROFILE
if [ -n "$GITHUB_CLIENT_ID" ] && [ -n "$GITHUB_CLIENT_SECRET" ]
then
    echo "GitHub OAuth is enabled."
fi

# Set the Eclipse OAuth client id and client secret
echo "spring.security.oauth2.client.registration.eclipse.client-id=${ECLIPSE_CLIENT_ID:-none}" >> $OVSX_APP_PROFILE
echo "spring.security.oauth2.client.registration.eclipse.client-secret=${ECLIPSE_CLIENT_SECRET:-none}" >> $OVSX_APP_PROFILE
if [ -n "$ECLIPSE_CLIENT_ID" ] && [ -n "$ECLIPSE_CLIENT_SECRET" ]
then
    echo "ovsx.eclipse.publisher-agreement.version=1" >> $OVSX_APP_PROFILE
    echo "ovsx.publishing.require-license=true" >> $OVSX_APP_PROFILE
    echo "Eclipse OAuth is enabled."
fi
  1. Build and run all components
  2. The application should run without errors and on the UI no PUBLISH and Log In buttons but it's possible to publish extensions via ovsx cli

@svor svor mentioned this pull request Dec 4, 2024
Open
@amvanbaren amvanbaren mentioned this pull request Dec 5, 2024
Merged
@svor
Copy link
Contributor Author

svor commented Dec 6, 2024 •
edited
Loading

@amvanbaren Here is my response to the comment #1056 (comment)

the Publish and Login buttons would not be displayed in the UI by default

No, that would be a breaking change. open-vsx.org and other deployments expect the Publish and Login buttons to be displayed by default.

The configuration for open-vsx.org and other deployments includes OAuth2. The UI will remain unchanged since isOAuth2Enabled returns true - Publish and Login buttons will be displayed by default as usual.

But for the new instances only if they will be configured without OAuth2 configuration, Publish and Login buttons would not be displayed in the UI by default. And it makes it possible to run OpenVSX instance without dummy GH OAuth2 configuration.

About a slight glitch, I tested current changes on gitpod and this is what I see:
When OAuth2 is configured in application.yaml

no_oauth.mp4

When OAuth2 is NOT configured in application.yaml and application-ovsx.properties

no_oauth.mp4

How can I switch to the dark theme?

@amvanbaren
Copy link
Contributor

How can I switch to the black theme?

By selecting dark mode in your browser.

@amvanbaren amvanbaren self-assigned this Dec 9, 2024
@svor svor force-pushed the sv-oauth-optional branch from 821acc9 to 08da93d Compare December 10, 2024 13:11
@svor
Copy link
Contributor Author

svor commented Dec 10, 2024

By selecting dark mode in your browser.

I can see the glitch after refreshing now:

screencast-nimbusweb_me-2024_12_10-15_15_52.mp4

I'll try to find a way how to fix that.

@amvanbaren do you happen to have any suggestions or ideas?

@svor
Copy link
Contributor Author

svor commented Dec 10, 2024

@amvanbaren I ran the OpenVSX registry without my changes from the master branch on gitpod and I see the same behavior. It seems the issue is not caused by my changes:

screencast-nimbusweb_me-2024_12_10-16_33_57.mp4

@amvanbaren
Copy link
Contributor

@svor I've narrowed the scope of this PR by checking if a user can login instead of whether oauth is enabled.

@svor
Copy link
Contributor Author

svor commented Dec 11, 2024

@amvanbaren Thank you, changes look good!
I've deployed registry on OpenShift and tested it with and without GH OAuth2 configuration:

Without OAuth:
screenshot-github_com-2024_12_11-13_14_48

With OAuth:

screencast-nimbusweb_me-2024_12_11-13_59_47.mp4

@svor
Copy link
Contributor Author

svor commented Dec 18, 2024 •
edited
Loading

Hello @amvanbaren,
Can this PR be merged?

@ibuziuk
Copy link
Member

ibuziuk commented Jan 7, 2025

@amvanbaren @kineticsquid @deboer-tim hello, could you please review the PR? this is the next step for supporting openvsx on-prem

@deboer-tim
Copy link

LGTM but I'm not a reviewer for this repo.

@ibuziuk
Copy link
Member

ibuziuk commented Jan 27, 2025

@amvanbaren @kineticsquid hello, the PR is sitting there for almost 2 months, could you clarify what is the plan at this point? Do you expect any changes or can it be merged? Thanks

@amvanbaren
Copy link
Contributor

The plan is to release this feature in v0.22.0. Release v0.21.0 is currently blocked. You can read more about it here: https://gitlab.eclipse.org/eclipsefdn/helpdesk/-/issues/5533.

This PR needs a final review, but I don't expect any changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@amvanbaren amvanbaren

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@svor @amvanbaren @ibuziuk @deboer-tim